Skip to content

P3481 R2 - Incorporate changes from the "bulk issues" paper #1500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Apr 29, 2025

Conversation

lucteo
Copy link
Contributor

@lucteo lucteo commented Mar 24, 2025

See http://wg21.link/P3481R2

Main changes:

  • add bulk_chunked and bulk_unchunked
  • bulk takes an additional execution policy
  • bulk is specified in terms of bulk_chunked

Copy link

copy-pr-bot bot commented Mar 24, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@lucteo
Copy link
Contributor Author

lucteo commented Mar 24, 2025

@ericniebler : this is not yet ready, but I wanted to push it up, so that you have a chance to see it earlier.

I'm planning to change the schedulers (static_thread_pool, libdispatch, parallel_scheduler) to be customized in terms of bulk_unchunked.

I'm also expecting some issues on the CUDA sides; I'm not sure how can I move forward on that side. Any help is greatly appreciated.

@ericniebler
Copy link
Collaborator

/ok to test

@lucteo
Copy link
Contributor Author

lucteo commented Mar 25, 2025

The GCC failure should be fixed. I'm not sure how to approach the Windows failure :(

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

@lucteo you can disregard the GPU CI failures. they are infra-related. the msvc failures are real though.

@lucteo
Copy link
Contributor Author

lucteo commented Mar 26, 2025

@ericniebler : there are some issues on the GPU side as well caused by my patch. I will temporarily pause this for 2 weeks, as I need to finish my ACCU presentation for next week.

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

ericniebler commented Mar 31, 2025

@lucteo i have addressed the GPU build problem. the msvc build failure remains.

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test

@ericniebler
Copy link
Collaborator

/ok to test 67740ab

@lucteo
Copy link
Contributor Author

lucteo commented Apr 24, 2025

@ericniebler : can you please retrigger? After a long and painful process I manage to isolate the Windows failure to an MSVC bug (see https://compiler-explorer.com/z/Eneab1zYT and https://developercommunity.visualstudio.com/t/noexcept-expression-in-lambda-template-n/10718680)

@ericniebler
Copy link
Collaborator

/ok to test 8e9547c

Copy link
Collaborator

@ericniebler ericniebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor suggestions. this looks fantastic.

@lucteo lucteo requested a review from ericniebler April 27, 2025 18:02
@ericniebler
Copy link
Collaborator

/ok to test a6dfb60

Copy link
Collaborator

@ericniebler ericniebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s'thing must've been amiss about my suggested edits. tests are failing now. :-/

Move the deprecated definitions in the base class, to avoid some weird shadowing error.
@lucteo
Copy link
Contributor Author

lucteo commented Apr 28, 2025

The error is strange. First, it works perfectly fine on my Mac; I was expecting the CI Mac build to be similar to my builds, but apparently it isn't.
Second, it seems that only the first declaration of operator() from the base class is used in the derived class, while the second is not. I find this strange (especially that it happens on 3 compilers)

Fixed by moving the deprecation in the base class. The deprecation method should also apply to bulk_chunked w/o policy, but there shouldn't be any code affected by this, so, we should be fine.

@ericniebler
Copy link
Collaborator

/ok to test a78089f

@ericniebler ericniebler merged commit 0242ad9 into NVIDIA:main Apr 29, 2025
18 checks passed
@ericniebler
Copy link
Collaborator

finally merged. thanks, @lucteo.

@lucteo lucteo deleted the P3481R2_bulk_issues2 branch April 29, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants